Support multiple meshes in R2S calculations#3860
Support multiple meshes in R2S calculations#3860eepeterson merged 5 commits intoopenmc-dev:developfrom
Conversation
eepeterson
left a comment
eepeterson
left a comment
There was a problem hiding this comment.
Thanks for the helpful improvement @paulromano. A number of small comments for your consideration, but nothing blocking.
| model : openmc.Model | ||
| OpenMC model object. Must contain geometry, materials, and settings. | ||
| domains : list of openmc.Material or openmc.Cell or openmc.Universe, or openmc.MeshBase, or openmc.Filter | ||
| domains : list of openmc.Material or openmc.Cell or openmc.Universe, or openmc.MeshBase, or openmc.Filter, or list of openmc.Filter |
There was a problem hiding this comment.
This list feels like it's growing a bit long and having the general openmc.Filter and list of openmc.Filter in the documentation may be slightly misleading since not all Filter objects should be viable here.
Makes me wonder if it's worth creating an openmc.Domain enum-like type that does the type checking and validation for us and leveraging that here (and elsewhere potentially). The arguments to domains are defining spatial regions regardless of whether it is implicitly defined through something else like Material assignments. It might be nice to have a type or class that checks that what's passed is a valid way of defining some spatial domain.
It may be obvious what should and should not be allowed if someone is using this function, but it might help to be more explicit.
There was a problem hiding this comment.
If you look at the function signature, we actually do have a DomainTypes type alias that is used for the type hint. When I originally added that in, I opted not to put domains : DomainTypes in the docstring though because it is not as obvious to a user what that means, and I figured listing them out explicitly is, well, more explicit. However, the DomainTypes alias is nice as it simplifies the signature and would allow a type checker to validate arguments.
There was a problem hiding this comment.
Reading this again, I think I misinterpreted your suggestion. Your proposed Domain is indeed different than the DomainTypes type alias (which is solely for type hinting). Adding Domain would be a bigger change that should probably be considered separately from the change here.
| domain_filters = [openmc.CellFilter(domains)] | ||
| elif isinstance(domains[0], openmc.Universe): | ||
| domain_filter = openmc.UniverseFilter(domains) | ||
| domain_filters = [openmc.UniverseFilter(domains)] |
There was a problem hiding this comment.
If we do move forward with the above suggested abstraction this code could migrate.
| for flux_arr, rr_tally in zip(all_flux_arrays, rr_tallies): | ||
| flux = flux_arr | ||
| reaction_rates = rr_tally.get_reshaped_data() | ||
| reaction_rates = np.moveaxis(reaction_rates, 1, -1) |
There was a problem hiding this comment.
could be worth adding same comment here as above in the flux loop to make clear what is being aligned.
| ---------- | ||
| domains : openmc.MeshBase or Sequence[openmc.Cell] | ||
| The mesh or a sequence of cells that represent the spatial units over | ||
| domains : list of openmc.MeshBase or Sequence[openmc.Cell] |
There was a problem hiding this comment.
Actually seeing here that we use domains for a few different things, that may negate my previous comment, but I'll leave it for posterity.
| if mmv_file.exists(): | ||
| self.results['mesh_material_volumes'] = \ | ||
| openmc.MeshMaterialVolumes.from_npz(mmv_file) | ||
| mmv_files = sorted(neutron_dir.glob('mesh_material_volumes*.npz'), |
There was a problem hiding this comment.
might be worth including digit specifiers in the glob string to ensure what you're getting is indeed what you intend to sort. Same goes for below in the photon glob search.
There was a problem hiding this comment.
Yeah, when I first wrote this I did have it with the digit specifiers but then changed it to the more generic expression for backward compatibility (since files created before this change won't have the digits appended).
|
@eepeterson your comments have been responded to/addressed. Let me know if you have further requests! |
eepeterson
left a comment
eepeterson
left a comment
There was a problem hiding this comment.
Nothing left from me, thanks!
2 participants
Description
This PR extends
R2SManagerto accept a list of meshes (as thedomainsargument), enabling mesh-based R2S calculations to use multiple non-overlapping meshes in a single workflow. Each element-material combination across all meshes is treated as an independent activation region. This can come in handy in a variety of situations:Checklist